WIP: Add typography performance tests#8918
Conversation
ksen0
left a comment
ksen0
left a comment
There was a problem hiding this comment.
Thanks for the work here! Comments below based on going through this PR with @limzykenneth .
The priority, especially if full completion is out of rant scope, is to complete the harness. If not possible to complete all the functions, these can be left open to community implementation in a new issue.
If possible, in addition to the minimal performance testing doc, it would be great to add instructions + example code for running manually (via CLI not the editor) a comparison between v1 and v2. As discussed, there is no equivalent quite for v1, but enabling comparison between v1 and v2 (even if manual / documentation primarily) would be ideall
| @@ -0,0 +1,65 @@ | |||
| # Typography Benchmarks | |||
There was a problem hiding this comment.
Thanks for including the WIP! In general the results do not need to be checked in. The documentation part can be put into a contributor doc on performance testing. (eg, here is the doc on unit testing, which is then included on the website)
|
|
||
| Implementation notes: | ||
|
|
||
| - All pre-existing benchmarks included the full p5 instance setup which adds ~50ms to each test. The `textToPoints() single word, isolated benchmark` test is included as a potential way to isolate performance testing away from setup overhead. |
There was a problem hiding this comment.
Same here: con be added to a new contributor doc on performance testing. That can be very short, so not necessary to add more than what you have here.
| options | ||
| ); | ||
|
|
||
| // TODO: This is an example of using setup() to isolate benchmarks away from p5 instance setup code |
There was a problem hiding this comment.
Possible to do everywhere? (Alternatively to upping iterations)
There was a problem hiding this comment.
I think even if we up the iterations, we'd be including the time it takes to create a canvas and parse the font, so ideally we'd go with this solution everywhere and move the setup out into its own thing
There was a problem hiding this comment.
@ksen0 @davepagurek Great, I’ve also parameterized the textToPoints() tests so they don’t drift over time or across the 3 renderers. The alternative is a lot of error-prone copy-paste test writing 😄
| options | ||
| ); | ||
|
|
||
| // TODO: textToContours() |
There was a problem hiding this comment.
If these are out of scope, rather than leaving them as inline todo's, you could make an open issue.
shakeelmohamed
force-pushed
the
bench-typography
branch
from
931e807 to
1a4fb64
Compare
4 participants
Add initial performance benchmark tests and structure for typography tests.
PR Checklist
npm run lintpasses